-
Notifications
You must be signed in to change notification settings - Fork 167
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
mark openssl classes as shareable when frozen #803
base: master
Are you sure you want to change the base?
Conversation
61222d9
to
d647710
Compare
d647710
to
70a025e
Compare
Unfortunately, it's not that simple. Some more changes are required before we can safely set the flag. Currently, quite a number of methods in ruby/openssl don't take into account the frozen state of the Ruby object. This is a bug which should be fixed anyway. But simply inserting a What makes it tricky is that some OpenSSL objects are reference counted. We can create multiple Ruby objects wrapping the same OpenSSL object, so whenever making one of them shareable, we must somehow ensure all Ruby objects are frozen. p1 = OpenSSL::PKey::EC.generate("prime256v1")
cert = OpenSSL::X509::Certificate.new.tap { |x| x.public_key = p1 }
p2 = cert.public_key
# p1 and p2 wrap the same EVP_PKEY object Since most of referenced counted OpenSSL objects have fields to store arbitrary data, it should be doable, but it's not a trivial task. Some objects can't be made shareable. One example I can think of is |
Nothing is ever that simple with openssl 😂 I'm listing here the changed classes, accessing for each, and can, depending on feedback, break it down into smaller PRs / add missing details: OpenSSL::BNThere are a few functions ( a = OpenSSL::BN.new(2).freeze
a.set_bit!(3) #=> #<OpenSSL::BN 10>, should have raised! In order to comply, a few OpenSSL::CipherThere are many functions here which change internal state, which require a rb_check_frozen About it being shareable, that's harder for me to access. It all falls down to OpenSSL::ConfigThis seems like a no-brainer: frozen when shareable (whenever initialized, its state does not change). OpenSSL::Digest/OpenSSL::HMACThere does not seem any value on making them shareable. It should probably not be possible to freeze it either. The latter does not seem to be enforceable, ,so it should at least be impossible to do OpenSSL::NETSCAPE::SPKIWhen frozen, assigning public key should raise an error. Freezing it should (probably) also free the key. Can't find any docs on whether OpenSSL::OCSP::Request/Response/*When frozen, none of the OpenSSL::PKCS12No setter, should probably be frozen and shareable by default. OpenSSL::PKCS7Setters and methods such as OpenSSL::PKeyWhen frozen (and as there are no setters), should be considered safe to use across threads: Both OpenSSL::PKey::ECtricky 😂 can the same as above apply? (minus setters) OpenSSL::Providerfrozen when shareable (no setters). OpenSSL::SSL::SSLSocketProbably not shareable, like TCP/UDP Sockets aren't? OpenSSL::SSL::SSLContextAlready freeze-able. Perhaps due to above, not shared? OpenSSL::SSL::SessionShareable when frozen. OpenSSL::Timestamp::Request/ResponseShareable when frozen OpenSSL::X509::Attribute/Certificate/CRL/ExtensionFactory/Name/RevokedSetters raise on frozen. Shareable when frozen. OpenSSL::X509::Store/StoreContextSetters and WDYT? while reading over docs about openssl, the general idea I got was that, once configured, most objects can be used safely across threads. |
Looks good to me: #808
Agreed, same with
It does have setters if ruby/openssl is compiled with an older version of OpenSSL. Since However, it seems unlikely someone using Ractors uses such an old version of OpenSSL.
I think so. Frozen socket doesn't seem useful.
I don't have an immediate idea how this can be done.
I think so.
I added a comment in #807, but this is tricky... |
And freeze
OpenSSL::SSL::DEFAULT_CERT_STORE
by default.Closes #521